 Hi everyone, welcome to this slightly different stream. It's also a different time of day You can't tell this if you're looking at this recorded But I decided to do a stream sort of impromptu and on a a time that's more in line with like, you know Asia Pacific time zones because I feel bad for you all you don't get to like actually participate live so I wanted to try that and The impromptu stream I wanted to do is basically you know, I maintain a decent number of different open-source libraries at the moment and I've gotten some requests up over You know the the months and years of people just being like I want to know what open-source Stuff is like, you know, it's a little weird to get into that space because you don't really know You know, how am I supposed to review a PR? How am I supposed to work with issues and it's hard to get that experience without already being in that position so I wanted to do a stream where I just Basically go through the sort of process of what I do when I deal with my github notifications This is not gonna be a very formalized stream. Like I'm gonna be watching chat a bunch I'm gonna Sort of just be walking through these one at a time chat of whatever comes up I don't really know how this is gonna turn out or whether it's gonna be interesting, but we'll see If you know, I mean if you're watching this on recording It's not gonna you're not gonna get a lot out of the the questions But hopefully chat is gonna ask the questions you have about you know Why are you looking at this notification first or why are you responding to this this way? Why are you doing the review this way? I'm really trying to give as much insight into my like code review and issue triage processes I can So chat this is this is on you. You got to do the work for everyone watching afterwards and hopefully it's gonna be interesting So without further ado, let's dig into Github notifications. So as you can see I have This is not a lot of notifications, but I have a page full of things that are unread. I have more but we're gonna ignore those for now Part of the reason why I have a bunch of these where some you see are fairly old is because I don't Often, you know after a day of work during programming I'm like, okay, I need to you know eat dinner and be with my girlfriend and play video games and stuff And so I usually tend to do my open-source work on weekends It's something, you know over time I'm hoping this is something that I can change where I can do more of it just as part of work for example But for the time being at least this is entirely, you know My thing separately from from anything else And so that means that in general what I do is I sort of clear through my github notifications on the weekend And then the next weekend there's another backlog of people who have responded or new PRs and whatnot and then I follow up on them from there I'm not gonna use the github command line tool for this because I don't Really like it all that much. I like the web UI for for issue triage and code review in particular I feel like it works just as well If there is something where I really want the like autocomplete I want it in my editor and then I'll just fetch it with git and open it locally I don't actually need the github CLI for that And same for merging like I'll just click the merge button. It's easy enough You'll see first that there are a bunch of PRs here some of which are fairly old that are specifically PRs to restation station So these we're not gonna merge in of these right now These are all upcoming episodes to the restation station podcast so if you don't know about this Restation station podcast is a podcast about the rust language where it's sort of a community podcast So we have multiple people contributing episodes Things like you know interviews with people doing cool things with rust or this week in rust or every 12 weeks Ben Striegel and I do an episode on the last two rust releases where we go through the change logs in some more detail and This is the github repo for that podcast And all of the episodes take the form of just a markdown file with some metadata And then it has you know a little bit of a custom format for indicating timestamps and stuff And so usually what happens with these is with the interviews in particular They'll be done sort of in batch and then this particular contributor will just like send me a bunch of like audio files and PRs with the show notes for those I review them a little bit and then merge them and that makes the episode live But because we don't really need to merge any of these now because the next one isn't gonna come out until Friday I'm just gonna mark this and as unread And and leave these behind You'll also see that I have some notifications here that are Left in my inbox, but are left as unread these are usually things where I know I'm gonna want to come back to them for Particular other tasks. I sort of have in my queue or There are things that I know I want to reference like in stuff that I'm doing at work for example or In other projects where I know that this is gonna be Relevant to something else that I'm doing But it's not actionable immediately by me. So I leave them as unread but still in my inbox and Then there's this one. So this I've had open for a while. This is basically someone who Watched one of the older import Rust streams on a bystander, which is this Lock free to wait free transformation library that we were looking at building I don't want to get into that because this one has it's a little bit more of like a Can you explain what's going on and why it does what it why the library or why the implantation in the stream doesn't do quite What we discussed so I will answer this at some point. Oh, it's been open for two weeks But it's it I'm gonna leave that in my backlog because it requires a little bit like longer of a process to write up a proper answer Let's see so the next one here this one is What is this So I made a change to Cargo a little while back. This is an unstable feature that is related to how Cargo passes rust flags to Build artifacts that are only for the host, right? So imagine you're running you're running cargo and you're trying to build You're essentially doing a cross-compile. So you have a target Specified on the command line like dash dash target, you know muscle or something But you're building on x86 linux There's a Anything that gets built as a build script is not going to be cross-compiled because build scripts need to be run As part of compilation which means that build scripts need to be built for the host platform not for the target platform but there's a little bit of Oddity in in the way that cargo handles cross-compilation in particular how it handles host artifacts So for example if you pass dash dash target and the name of the target is the same as the host platform Then the question becomes okay, should those rust flags apply to the build script Because the build script is being compiled for that target But there it's not being used for the target and this turns out to matter in a couple of cases And it turns out that cargo's defaults are a little weird here And it seems like the fix I made to that feature meant that rust doc flags are not being handled correctly This one oh interesting Yeah, as you see there's the referencing a PR of mine where Where I Fixed some parts of that feature but clearly not all of them I don't think I'm gonna dig into this one now because this is gonna require a little bit more research And it's also something that's Related to some stuff. I've been doing at work. So I'm just gonna handle that separately at work I don't think trying to debug this here would be super interesting anyway. So we're gonna skip past that one This is a PR to Hash bag so hash bag is a library I built that is like a hash set except it also keeps the cardinality the count for every item so you can stick a Value in there. I mean with sets they're all arguably keys But they're also all arguably values and you can stick a value in there multiple times And it will keep track of how many times an item has been added So, you know with a normal set if you try to insert a value into the set that's already there The insert does nothing it has no effect Similarly if you remove something from the set that was in the set previously will always be removed with this if you insert the Count for that value goes up by one and if you remove the the count the value the count for the value goes down by one And it only gets removed from the set when the count hits zero So this is someone who contributed a PR that adds the difference method to Hash bag so if you've looked on hash sets in the standard library Hash set has this Difference method where you give it you have the current hash set you give it a reference to a different hash set and it Gives you I think difference is an iterator Yeah, it's a lazy iterator that produces the elements that are in the first set But not in the second set and similarly it has operations for Intersection union and symmetric difference We don't have this for hash bag Partially because it's it wasn't something I needed so I didn't build it and partially because and you'll see this when I go Through the PR a little bit It's not entirely clear what the semantics should be. Yeah, it's a it's a multi set Or also referred to often as a bag So This person submitted the PR two weeks ago and I responded saying I wonder whether we want difference to produce the count of The difference and not just the set of items in the difference. So the original PR was That hash by difference exactly like hash set difference returned an iterator that told you things that were in the first set But not in the second, but I or rather I think the original one was Was not the count but was just Where the count in the first set is Higher than the count in the second set But it only returned not the difference in the count But the values for which that was true and my observation here and you can sort of read through the discussion I'll put the the link in chat here And The discussion here is Whether Which of these is better right should difference match the API that's in the standard library for hash set Or should we have our implementation for difference? Give a different iterator value that includes the count or should we have you know match the the standard library? Hash set difference and then have a separate method to get an iterator that includes the count and so that's been the discussion that we've gone back and forth on a little bit and My argument for why we should probably have it be have it include the count difference in the iterator is because It's pretty easy to remove that like you know, it's just map it out of the iterator But it's You know if you take the one that doesn't have the count you can't add it back in and so chances are or not chances are like Logically one is just strictly more powerful than the other one So I feel like that one should get the more convenient name And at that point why even have the one that only returns the the set difference and There's a second question here of at that point in order to compute the bag difference So the difference including the counts you basically have to construct a hash bag to compute that difference because you need to Keep track of the difference count, which is itself a bag a multi set So the question is should it return an iterator still or should it just straight up return a New hash bag that contains the difference And in fact this basically means you know that the difference ends up being the result of Subtraction and it returns a new hash bag and that that was sort of where I think we we landed with the last piece of discussion And It seems the last update here from the other person is Played around a little bit more with getting the value and the count and it indeed wasn't so bad Let's go for that. I pushed commit now to make it so alright, so let's go and look at the difference So it's at this adds a difference method to The hash bag I don't think the explicit lifetimes are Relevant here, so let's go ahead and say That this and this I Think we can rely on inference here and not use the explicit I have time syntax So we'll start a review here because there might be other comments too if so there's sort of a question of at what point Do you approve a PR if if this is the only comment? Like as I go through I would probably approve it because this sort of causes churn, right? It means that realistically this is not going to get merged for Like probably a week right because I leave this they won't see it until you know in a couple of days Maybe then submit their change and then I won't review it again Until a week from now and maybe then I'll merge it So if this was the only thing this is not worth blocking for in worst case I can always just override the change myself and so I don't really want you know if this knit was the only thing to Comment on that doesn't seem worth blocking anything Oh, you're right. It I mean it is inference like a lesion is inference But I think we can use They're sort of equivalent here Yeah, so so someone pointed out too Well, we'll get to that when we look at this later whether the count should be a u-size or an i-size here and It's another good question where In order to compute the difference we probably do need to keep Well We shouldn't need to keep negative values, but we are always going to compute the difference even if it is negative so the question is just just just Return a hash bag that includes negative Cardinalities like never negative counts or should it filter out all the ones that are negative? If it does include things for the negative count then it needs to return an i-size But if it's going to filter them out it should return a u-size I'm not entirely sure here. What is better? There is the argument that if we return the the counts and allow them to be negative then You know that then this we don't need a separate API for inverse difference And you know you get more information out of this one API But it also means that anyone who actually just wants the difference is going to have to do the filtering themselves Which is you know the most common use presumably of a method called difference I do so It looks like difference stores others. I don't think you can alight the lifetimes That's interesting. I mean this is this should be easier to test out So if I do you know fn foo, no I do struct foo I do info foo Just so we don't give them an incorrect recommendation bar if it takes self and Other which is a reference to self and then it returns a Self so here. I'm gonna use a lesion And I do something like you know if true then self Else other Let's see what Ross says about this. I guess you're right. Yeah, cuz you I think the rules for elision You know, you're totally right. The rules rules for elision is If there is a self then you self Otherwise use the combined lifetimes of all the arguments that are references So now you're totally right Which means this comment is simply false Nice good catch Well, so so someone said the name is different so I size would make sense But the the documentation for this is returns an iterator that visits all values present in self that it is not present in other This it's also unclear whether that should be the documentation, right because Arguably Arguably it doesn't return The ones that are not in other right not present is not the right phrasing here The right phrasing here is something like returns an iterator the visits all values present in self That I mean that's also a question of semantics, you know, should it should we Say that it returns anything it Returns the count for anything that's not in other or should we say it returns How many more there are of anything that there are more of in self Right, both of these are valid interpretations of difference. I wonder what the Sort of set definition like the mathematical set definition of multi set intersect or multi set difference is That's a good question, right? Like what should the semantics here be should it be That to make it more concrete right imagine that that Imagine that the left bag left bag has a to be one and the right bag has a one and Only a one Should the different is the difference a one Be one Or is the difference a Just be one Because the right bag did have a its count was just different Someone has the answer for us. What does Stack Overflow say? This is gonna be bright. I think maybe Okay, there's a lot of pop-ups here. Yeah, exactly. So this is that same question of you know Both a and b contain one so even though the count is more in a is it still a result in the in the output Interesting That's good. I'm not sure there's a universally agreed upon definition of set the erratic difference for multi sets This is not where I wanted to go with stream, but that's interesting enough Doing so this definition would yield Yeah Yeah, I think I agree that I would expect it to return the difference in count The problem right is it's not strictly more powerful because I if I give you If I give you an API that returns this You don't have a way to turn it into this Because you know that the count is one it's indistinguishable from this be one unless you do something with it in Relation to the original sets So I think there's an argument for there really needs to be two different methods One and someone pointed this out to one of like set difference and one for bag difference and The question, you know is what should difference do You know if we could just say we don't want to have a difference method because the answer is non-obvious Right, so that this is sort of the principle of least surprise right where If it's not clear What the semantics of a method should be then you shouldn't have that method you should have two methods with Clearly defined names that are Distinguishable based on their names because otherwise people are gonna guess and when people guess they will guess wrong some of the time So I think the way I'm gonna approach this is Something like This comment sent me down a rabbit hole of Trying to figure out what difference what the semantics of difference for Multisets even should be This Stack overflow article or Post Suggests that both that given a I guess we could do 112 minus B One Can feasibly be the given a equals this and B equals this The intersection a Dot intersect B Confesably be both One two and Two Since that means that the word Difference here is ambiguous. I think we should instead have two methods What should we even call them? So someone suggested Set difference and bag difference and I don't know if that's the way to think about it Right because okay, let's say I told you this takes the set difference Does that mean it like if that's all I told you does that tell you which of the semantics you get I don't think it does So I think what we want to call this is something like One option is something like count difference and Unique difference is not bad Alternatively Okay, so here's another to take it very differently One is count difference Or and the other might be But not in It's sort of an awful name, but if you think about it in terms of what you would write You would write a Dot but not in B Which isn't awful Because it clearly is anything that's in a but not in B Lazy and greedy difference. I don't think helps here Even even not in yeah, even not in is not bad Yeah, like multiplicity Cardinality maybe Collecting the hash back into a hash set. So so I mean we could tell the user we only support one of these And then we tell them, you know for each hash bag turn it into a hash set and then take the difference If what you really want is the set difference, but that's not very satisfying because it means you have to allocate Basically three hash sets because you have to take the hash hash bag a and turn it into a hash set Hashtag be a set hash bag B and turn it into a hash set and computing the difference of sets For sets, I don't think you need a bag. So for set you might not need the third allocation So so one problem with count difference is that it might do C1, you know, there's there's an argument that it should return A1 B1 C minus one you're right. Sorry. I meant I meant difference The difference oh You're right multiplicity is the number of occurrences of an element cardinality is the sum of the multiplicity So basically the size of the bag Yeah naming is hard and you know, this is one of the things that that's hard about maintaining libraries is it's You know, this doesn't happen all the time But you do get a decent amount of contributions where the difficulty is not so much in the implementation It's in terms of what is the right way to land this feature that is both, you know Good for the users of the library and something that I can maintain long term and that I don't think I'm gonna have to break in the future Yeah, so so the argument here is count difference could arguably be interpreted as this Although and I think this is more like compare than it is difference because when we say a minus B Which is sort of what difference means right a difference is subtraction here arguably and a minus B will never include C Right, it's remove anything that is in B from a Which shouldn't include a negative count for C. So I think we don't want I Don't think we're gonna want negative counts. I think that's right Yeah, exactly if you were loud negatives this is arguably just a different method So I think what we'll do is I think we should have two methods So there's one that's not including or Subtract subtract helps compared to difference because I Think it's clear that if you subtract it you really mean remove those elements. There's even you know It could be removed But that seems weird But I kind of like subtract it's fairly straightforward, you know, it's not a super complicated word Yeah, let's let's do subtract and not in oh without is not bad a Without B It I don't I think without has the same problem as difference. It's not clear which of these outputs you would expect with without Key difference and value difference Mmm It's not really key and value in a bag though like it's not as though the count is the value and the key is it's not as though the key is the the I The element and the value is the count that is how it's implemented, but it's not really how the the Interface talks about the type This is now diverging from hash set difference But I don't because this is a bag it has different semantics I actually don't think we should use the word difference because I don't think it I Think people are gonna assume that it means the same as in the standard library and it just doesn't directly translate Yeah, I okay Let's do this and then the other thing that I like doing here is you know It's easy for me to go as a the owner of this library to just be We're gonna do this, but I don't want to do that here because First of all, I don't want to tell them you need to implement both of these methods It could be that they only need one of them and therefore You know, I don't I it's okay for me if they change this PR to just have one of them So what I'm gonna do is I Hope hopefully it's clear which is expected to have which outcome Otherwise these names are bad too I'll make that not a parenthetical because it's so long What do you think happy to have this PR just Include one of them If you don't want to Implement to the other Yeah, the goal here, right is for both is for the names we choose to be unambiguous They might not be great names They might not align with other APIs, but the chances someone guesses and guesses incorrectly is what we want to get at, right? Someone's suggesting in chat that we could use the semantics of joints here from relational algebra, right? So you could talk about this in terms of like a left join or a right join It's not awful because like relational algebra is basically Bags like that's sort of kind of what relational algebra ends up operating on Is multi sets and so, you know, this this is arguably a It's not a join though is the thing because you're it's a remove a join which I don't know if there's a an expression for Like what's the inverse of a join in in sequel, right? It's this is it's not a join if it was a join a join is more like the intersection It's an anti-join. Yeah, but I don't really I I don't think I want this to be anti-join I don't think that helps So let's leave that as subtract and I'll Start a review with that And then I guess here Once we've chosen The semantics this function should Implement I think we should also update the docs here to Specifically mention The interaction with counts right so here as an example it's Way, why is this constructing a hash set? Oh, I see, okay So if we have one two three three and return and B is two three Then this API expects that the difference is one one and three one Okay, so this is subtract if this was not in Then three wouldn't be in the output because it's also in B So instead of being cheeky over here. I'll say I believe this would be subtract To be more helpful Frognify, yeah, that's always a good fallback Great Yep, that's fine The bag with entries we do not want to return upper bound for that that's fine That's also okay, why not derive debug here Because he's trying to hide the upper bound field That's probably fine. I think I agree with this, you know There's a there's an argument for the code is simpler if we derive debug But I don't I think it's actually nice to remove the upper bound here It's it's just noise in the output It does mean that if we add other fields to the difference in the future the debug will have less info But but I think cleaning it up now is is is worth doing and then iterator for difference It grabs the next from items It looks up that element in the other. Oh Right, so it doesn't need to keep a bag of the difference Which makes sense because we keep it as a bag we we keep the count per element so you can just look it up directly And so this is if other is less than n then return the difference Yeah, in which case this totally should be U size and this is indeed subtract whereas for not in this would be Oh, that's a good question actually if I go to hash bag So this has Contains which returns a U size and it has right so for if we wanted to do this with No, we could still use contains and just check if the value is Zero in the other set that would be the only case in which we return it from In which case we've returned T and and I think for not in the return would actually be a set It would not include the count. So that's another thing we should add to this comment So and notably Not in Iterator would not include account or Or Well, I'm less sure about that actually Okay, so this is another question about the API, right? So let's say we're looking at the not in method Here should it return B1 or should you just return B I Guess it might as well just include the count Yeah, I'll leave that in it's fine. It can keep a Well, no, right? So you can imagine that if B here was like 14 then The 14 here does carry meaning right if we if we instead returned a set of just be it doesn't really help You know it it just removes information. So I don't think there's a big reason to not include the count there All right, that's fine This makes sense looks good. I like the question mark operator here although This doesn't seem right Because the upper bound for size hint should go down as we return elements Because we know that you know if we know that the thing is gonna return It's if we have a bag of n elements We know that the upper bound initially is n but the moment we remove the moment we return an element We know that that value is lower. In fact, every time we go through this loop We know that the upper bound is one lower. So I think there's a missing Upper bound check here. I like the tests That's beautiful Do test difference that seems fine So what where's the place we could constructs this? Yes, the upper bond is initially set to the count so I think here I Think This needs to this is another thing that I try to do in in PRs is You know, keep in mind that the other person the person on the other end of this PR You know, they both have other things to do But also if all you give them are critical comments and like stuff that's just like this is wrong It's really demotivating. I don't know whether this is like this person's first PR or whether they've done a bunch before And so I want to try to be encouraging for these and not be like you're dumb stupid Because that's not really the reality of the situation like this is a bug. I could totally have done myself too So I'm gonna go ahead with I think this needs to also Reduce the upper bound for each iteration all the It's not Incorrect not to do so But since the iterator can be inspected at any time we should Tighten the bound as we can and Again, like it is true that it's not incorrect size hint is supposed to return an upper and lower bound But it doesn't have to return the correct up like it doesn't it's not expected to actually return the exact size And it has to be an upper bound here for bag because we don't know how many more elements We're gonna return it could be that for all of the things that we have remaining in the in you know a They're all in B with higher counts So even though the upper bound is the size of a or the remaining size of a that we haven't iterated over It could be that the actual number of items returned is zero So this is why we can't return we can't implement exact size iterator for this because we don't know the result until we've computed Oh, we got the the prim again in a year. That's exciting. Hello Oh Apart from that, I think I'm pretty happy I'm gonna go ahead and also leave positive comment because I reacted positively to it The primogen the premier again the prime me again from me all got going in record That rhyme agent It's unclear unclear unclear how to pronounce primogen primogen Did I get it right the primogen? Does it have to be said with like a fierce like movie trailer voice the primogen? Great so I'm gonna do that and I'm gonna leave this as a request changes Fantastic. All right. We're through one PR someone asked and I think it's a I think it's a Good question of you know, this is a pretty thorough review that we just did for a relatively small change And it's not always that I'm this thorough or rather It's not always I have this much to say or this much to think about but that's sort of what I wanted to point out here is Sometimes simple changes require a lot of thought from the maintainers point of view because even though the changes straightforward The implications of it are not always sometimes. I'll see a change and it'll be a large change It'll be sort of obviously correct or it'll be something that we've already talked a bunch about so it's it's clear This is the right approach in this case the small change but whether the semantics that change of right aren't clear and that's why it's it's very hard to anticipate how long it's gonna take me to to get through my notifications every week for example because sometimes I'll have like three notifications, but they're gonna take me like all day to get through But yeah, I do think of these PR reviews as more in terms of it's like Asynchronous pair programming almost and I think that's a much healthier way to approach them, too Then like I'm gonna see whether your code meets my bar, right like that's not useful How do you feel about calling do test different so many times versus say taking a go table-based test approach Down here I like this I think this means that the tests are easy to read You know it could be that we can do a macro here with some nicer syntax, but I don't I think this is fine This makes it very clear what's going on from like, you know if we actually included the counts I think it might be even harder to read these, you know like Looking at the the set theory here like I think this was easier to read than if this said one colon two There's more syntax more noise. Sorry for the bright screen All right, so I'm gonna mark that one is done and we can move on to the next one Took up more time that I'd planned but such as life This is a PR that's landed ah So this is something that this is a PR that I went through a decent amount of discussion on It's not too much to say about it now, so it's just notifying me about some some follow-up comments Basically the change here was this is in the open SSH SFTP client. So this is I Maintain a crate called open SSH that implements The client side of SSH connections using the standard open SSH as the real client So the origins of this was it would just wrap Like Tokyo process and just shell out to the local SSH command the idea being that rather than implement all of the like crypto and stuff myself I could just Rely on the correct implementation of open SSH and get all the features like support for SSH agent and stuff for free and then Nobody zoo Jew Don't know how to read their name, but this person came along and it was amazing just came in and was like I'm gonna just implement the SSH multiplex protocol, which is basically the the or MUX protocol rather So this is the protocol that the SSH command line tool uses to talk to the local SSH Daemon that represents a connection to a remote host So that that daemon is the one that does all of the actual Cryptography and authentication and stuff and there's sort of a relatively simple protocol That's entirely local to your machine talking between the SSH client and that MUX daemon And this gives us a much nicer interface because it means that instead of like Invoking commands and trying to read like standard error or reading the exit codes and try to infer what went wrong We can actually talk the protocol and get real error messages back and stuff And they've taken that work a little bit further now and implemented the SFDP protocol or client fray as the SFDP protocol which runs over the SSH protocol and They've sort of ended up folding me into that work because it relies a lot on the open SSH Client that I built and in this particular Change was The stuff they built originally has like unpin bounce everywhere because it makes it a lot easier to Just get your code to work because you don't need to worry about pin if every type is unpin but It's a little annoying sometimes because sometimes you truly have a type that just isn't unpin like if you have a An async block for example, you don't want to have to box pin it So a lot of this changes is removing the unpin bound and in order to do that You need to make sure that the basically inner parts of your stack correctly pin values So that they can be not unpin. I know the terminology gets weird here But that's basically what's going on and the discussion that we've been having in this PR is basically whether Let me see if I can dig up Oh beautiful they added this that's great Let me see if I can find low-level pin utils They basically added a little bit of unsafe code to enable them to write this and part of the reason They asked for my review was like can you check that this unsafety is actually safe? Let me send the URL over here So this is We actually went back and forth on this because the initial implementation wasn't safe and so we We sort of went back and forth trying to figure out how can we make it safe? Is this like something that's missing from the standard library? How do we go about it and ultimately we landed on an API that ended up being a little bit more constrained But was actually safe and this was just a The last little comment here was a sort of particular little loose thread of this is the thing we should stop All right, that's unnecessary. Let's go ahead and do this and Go ahead and do that Bye Yeah, I got rid of them easy Yeah, so this discussion was just that there was a particular unsafe block in their implementation that was only safe if certain other things were true about the About the rest of the code and so I made the point that you should add a safety comment for this Explaining that this is safe because we don't do this anywhere else in the code and they did so this one can be marked as done That's great Let's go back here I'm not gonna review this particular one because well, let's open it and see what there is Future resume wait, I'm confused There are clearly some file differences that are not being shown here. Oh, it was just showing me the difference from last time Oh, this is actually not too bad So this PR is When you use open SSH to this way when you have this like local daemon and you have the the actual client that talks to the daemon The way the library previously worked was it will spin up the daemon for you for the particular connection to a host that you want and And then reuse that daemon for you know the duration of your program until you drop the session But when it drops the session it terminates the daemon There are some times when either you might already have a connection to this host and you just want to use this library to Continue using that connection or you want to leave the connection open so that a subsequent invocation of your program Can can pick it back up and so this adds the mechanisms required to do that through a leak function and I have some I don't love that terminology, but it adds a Function that lets you say when the session type gets dropped Don't close the connection. Don't close the daemon. Just leave it running And then as a resume which is don't start the daemon instead use the daemon that exists already on this local socket and so that's sort of the the primary thing that was being added here and This is a PR that I've been through a review of before Let's see I really hate that github will close all Comments that the other person has resolved because I might not agree with them for why they resolved the comment So I have to open all of them and look through and see whether I still agree. So here This was yeah, I don't want deny warnings and see I and they reverted that change great so I can get rid of that They fixed that I saw it This was in see I there was a sleep 30 Right, so this is because in order to run see I for this we spin up a docker container that itself runs an SSH server and That takes a little while to spin up before the SSH server is actually available. So I think the sleepier was added to Wait for the SSH server to be ready before we start actually running our tests and the problem here is we don't have a good mechanism for Wait until that other docker container over there in see I is ready to accept SSH connections And sleep 30 just seemed arbitrarily long and on the long side Sleep 15, I guess that's fine. I don't love the fact that it's asleep There's an argument here for maybe it should be like a Command that we run in a loop instead But then you run the risk of what if the SSH server never starts and now your see I just hangs forever Maybe that's nicer, but I'm okay with the the hacky work around here. So here's the argument for leak So the way this was set up was you have a session type that represents your connection to the the back-end Damon and The idea with leak being it consumes self and gives you back the information that you would need in order to resume later and that primarily means a path this is the path to the Unix socket the local Damon is running on that you can use to connect to it in the future and optionally a Path to the log file that that was open for the Damon which we might not know right if we have already if we're Resuming a path session. We don't know where that Damon keeps its logs. That's why this is an option and So my point there was I don't love leak because it already has pretty strong meanings in the Rust ecosystem and This ties back to the discussion we had on hash bag Which is when people see a name that they recognize and have a strong association with they tend to assume that it means the same thing It has the same semantics and leak in rust terminology means that the memory leak Right, it means that this thing is now going to be like it's a heap allocation And we're gonna forget that it was a heap allocation that we're not gonna you know free it ever in the future Which is not what this is doing that said, you know leak is kind of appropriate because that is what it's doing It's not gonna shut down the Damon for you. So I understand why the name was chosen But but I don't love it as a name and so what I proposed instead was that we instead have a configuration option for sessions that is like close on drop and you can set it to true or false and one of the reasons I like this is because The leak had to be a little bit special because Imagine that we are resuming an existing session that implies that we also should not shut it down on drop Right if if we're resuming a session chances are they wanted to continue to be resumable And so there was all this logic in here for you know If it was constructed through resume then drop does nothing if it was not then close the Damon And then leak had to do the same kind of checks So my proposal here was just have a close on drop That's a boolean that we used to determine in drop what to do So that way if you're resuming a session You still have the option of making us close it on drop or if you opened it and that started the Damon You still have the option of it not being shut down on drop and Then I proposed you know if you don't want that and again this comes back to I don't want to claim that I have The right answers here there might be other API decisions that you know this person who contributes to this and therefore presumably has a use for it Actually means that it really should be this way then I proposed calling it detach instead of leak Leak connection could work too I'd like to keep it as it returns the path to the control socket and the path to the math or log Which is useful for process MUX Done so I'm guessing that done means uh Here Yeah, so they renamed it to detach I Like detach I think detach sort of says Says what it does I do still kind of like the idea of also having close on drop as a separate thing Yeah, so force terminate was another thing where That they had for the same reason that if you resume a session then it wouldn't Close the daemon or shut down the daemon on drop But they had a force terminate that would when you call it would definitely shut down the daemon no matter how the session was started So let's see let me go back and look at what the diff now looks like there are some times when I Want to look at the full diff and not just the diff from last time because I want to get a sort of holistic sense for what the API looks like This sleep 30 to 15 that's fine Yeah, there's a comment here for wait for the startup. That seems great wait for the startup. I'm happy with that So here's resume It's fine that ignores that So close is the right because so the Normal implementation will or when you construct a session Which also has to start the daemon for you it constructs a temporary directory Which is where it sticks that socket file for the daemon and so if we if we created the daemon Then we know that tempter directory and then on drop we want to remove that tempter directory again Because we know we shut down the daemon as well So this is just having the conditional here now because now that it's possible to resume We might not know the temporary directory in which case we need to do this. So that seems fine Detach will just Take the path from the temporary directory without dropping it so tempter is a type here where It creates a temporary directory for you and automatically drops it again Deletes it again on drop and so this is a way to tell it. No, don't delete on drop That seems fine that's fine That's fine, and then I want to see what changed in the top level So you'll see there's like a session dot RS for the process implementation This is the thing that like forks out to the SSH command and the native mux implementation Which is the one that just talks directly to the socket daemon over the its own little protocol And so let's see what we have on session now So we have resume and resume mux. So this is the way that you choose which of those backends you want to use We have detach for saying Don't drop this don't Close the connection on drop And we have the tests down here. Well, I Am kind of sad that there's not a close on drop a session created this way We'll not be terminated on drop but can be force terminated by force terminate That doesn't sound right. I Don't think that function is there anymore Unless I'm blind, which I could be all commits So in session we have resume Yeah, see unresolved link to force terminate Yeah, I think there's something missing. I think they forgot to add close on drop Test force terminate and yet this doesn't call force terminate Something's something's off here Let's see if there's a more I Guess I can close this and I can close this Move its functionality. So what did this commit actually do? Oh? I see This doesn't seem right. So this what this is saying is When you call close it will always terminate the oh I see yeah, okay, this makes some amount of sense. So the API now is The API now is you can call close on a session and Because you explicitly called close it wasn't a drop It was explicitly close that will always shut down the the daemon and the ultimately the connection to the host If you just let it drop if you don't call close then the semantics are if it was resumed Then don't shut it down if it wasn't resumed like if we created the daemon We also shut down the daemon and so the idea here being the user Still has the option of if they want to leave it running They should always call the touch if they want to shut down they should always call close And if they just let it drop then we do what we think they probably meant I Think I like that. I think I'm okay with that it does mean though that This This doesn't seem accurate anymore since there's no force terminate now And same thing up here in the API where This should be called close now. I think and I think I want to add here too that We should also update the documentation on close to Mention that it will terminate the The it's called an SSH terminology, it's the control master That will terminate the control master Even if the session was resumed Chat says if close historically didn't do that you might want to hit a major change So historically there was no way to resume a session So close if you called close it would always terminate the session because that was the only semantic we had So this is not a change to semantics luckily Same thing with drop but the it used to be and still is the case that if we create the session Which is used to be the only thing you can do then on drop. We would also stop the session So there's no change to semantics here great Wouldn't something like exit be better instead of close because it's closer to the client's API You're not wrong like the command to the control master is exit not close But I don't know which is better here I think close is okay because the the way to think about this is you're closing the session Right the in terms of the naming of the type you're calling it on I feel like it's fairly clear whereas exit isn't quite clear like what is exiting the session mean Arguably that could mean detach You know it could be something like kill or Terminate but I feel like close is is semantically close enough so to speak Nice almost there I like the simplicity of where we Ended up Just some cleanup related to old references to force terminate left And request to change great. All right, so we did that one Update IO lifetimes requirement. Ah, so this is a PR from Dependabot also to open a stage and the change it makes is just that it bumps the major version of one of our dependencies from 06 to 07 and At the time it just sort of that looked fine And we just merged it without thinking too much more about it But then we realized that the IO lifetimes crate is Actually ends up showing up in our public interface for a real silly reason, which is we have this impulse So STD IO is a public type in the crate and we implement from owned FD for STD IO This from implementation is something that we use internally in order to Construct one of these from one of these but the construction happens internally So there's no real reason for anyone else to ever use this this from implementation But it is there so it's a part of our public interface and this type comes from The IO lifetimes crate and so what this technically means is that someone could have taken a dependency on this crate and also taken a dependency on the old version of IO lifetimes and Depended on and expected that one in the type from our crate implements from of their type So if we bump the major version of IO lifetimes their code might stop compiling Which you know would be a problem. It's a backwards incompatible change in practice. It seems Extremely unlikely that someone depends on this change partially because this is a very This was only introduced in a very reasoned version. So what we actually decided to do here was to Hide this from implementation. So make instead of implementing the from from trait which ends up being a public part of the API add a from method that is just Internal to the crate to STD IO that implement that takes an own FD your returns and STD IO That way it's not a part of our public interface anymore And I think we should just do this change because even though it's theoretically breaking in practice it's not breaking and It enables us to disentangle our major version from that of the IO lifetimes crate Which means in the future we can do more of these updates without it having an impact on us and nobody you agreed and filed this PR, which is probably going to be Simple looks like IO safety funds can be stabilized in 163 Let's look at the the changed file first. Oh I guess we weren't even using the from excellent It's not so someone said being able to create an STD IO from any FD might be a problem. Anyway, it shouldn't be in fact This is something we kind of want to support. It's just we don't want to tie that support to a Sort of a major version that we control don't control of a crate that You know is not essential to the API One of the things that we're gonna see is owned FD I think is landing in the standard library and that's what this This latest comment is about is that it looks like it's actually going to land in rust itself So if you look over here IO safety is an RFC that adds More do we have this Owned FD, which is that same type. This is basically what that crate was providing was a sort of prototype of what was eventually going to land in the standard library anyway and it seems like that has Landed and it's going to be released in rust 163 I think what I want to do here is actually to land this anyway, well It's a good question right so the question is Is it worth removing this from implementation? Just to add it back once this lands in the standard library. I think it is because currently it's a It would be a different from implementation like when this lands in the standard library It would still be a change to the API moving it to what the standard library provides Oh Is it safe to make a stood IO of a file FD or an EPOL FD? It's safe. It's just weird like it's not gonna you're not gonna get any input from it But it's I mean it's safe. It doesn't cause any memory of the unsafety. It's just weird So I think what I'll reply here is I'd like to remove it anyway in the interim Since as it stands it's It's a breaking change problem waiting to happen Happy to add it back after 163 without the dependency Um nice So I guess I can roll resolve that conversation. I'm gonna merge this and then what I'll do I guess I'll show you my release process for this So if I go to minor open a stage you pull Let me do fantastic Um, so here we now have that change Um What even changed in cargo tumble? It looks an awful lot like nothing changed in cargo tumble log dash p cargo tumble Oh, it's just this dependency update. Okay, that's fine. Um So if we have a change log here, which is kept in source change log Um And this is going to be 092 And what we changed was removed Uh Breaking change risk Uh Removed Imple from owned fd first at i o As it was an unintentional part of the public api Um This is technically a breaking change, but should In practice effect No one Uh, and then I'll update the cargo tumble change to this to 092 um So we'll commit that as release 092 and then I have this little script that is I find it really handy all it does it it walks back the git log of cargo tumble And looks at which commit changed the version number and then tags Um Each Commit with the version in which it was created So in this case, you see it found all the old tags and it tagged this latest commit that we just made with v092 um And then I just do Get push tags and get push and cargo publish boom and 092 is out fantastic Uh Shall we create a new release and you'll uh I think you mean yank all the other zero nine releases, um I It seems all an awful lot like this person is also on stream given how quickly they responded um Although I haven't seen their name in chat, but could be they are Uh So yanking is tricky. Um, I'm not a huge fan of yanking versions unless there's actually like a security problem with them because Yanking makes it so that anyone running an older version is forced to update They're not actually forced right if they have a lock file. They're just going to get like a warning and stuff but It creates friction for users that You only really want to impose if there's like You got to fix this or like You have a big problem on your hands things like, you know for if you have a crypto library and it turns out that like It was leaking your private key to a public github gist gist gist gist Good question, uh, like that's something I would yank. This I don't think is important enough to yank. Um I think this is quite worth the Frick the user friction of a yank given that there isn't actually um A security risk um to this The fact that There is a um A breaking change Is Mostly or is going to be mostly irrelevant to users Released as 0.902 Always with tada, it's important amazing This means we we closed an additional pr. So if we go back to the notification list now If I refresh it, uh This one's done We got we got more comments. Did chat start commenting on this? Someone for I think this is somewhere for chat I see you there. I see this username um self.count is a total number of items including duplicates And the difference iterates over Yep, I think you're entirely correct Good comment. I approve dr. Eamon dreamon Dr. Eamon dirt. How do you pronounce? A hyphen I'm gonna ignore that. Um Where we're not realistically going to get through all of these today. I have to sleep at some point uh Let's do something that's not open ssh Consider using text anchor for right to line strings So inferno is the port we did of um The flame graph library to rust from pearl And you know since we did the port we've actually made a decent number of changes to inferno that aren't in flame graph things like improvements to the generated uh svg and the embedded css and and javascript including the algorithm itself um I've used a somewhat modified version of the inferno crate to construct flame graphs for a blog post Made a couple of changes that I think some contributor here may be interested in implementing properly in the project proper um We have two strings to respect to do a line to the right side of the image Right, so it generates an svg that is a flame graph So this sort of stacked colored bar charts of where your program is spending its time um The search and reset search at the top and the matched statement at the bottom Right now they're aligned by giving them 100 pixels of space from the side Which can fail to fit the string. Yeah, this is the classic like let's not absolutely position things. That's a bad idea Instead of consider using text anchor end. I didn't even know that this was a css Specifier that's cool placing the text node at a constant location where the string should end Oh, yeah, that's a great point Uh, I agree that seems Like a good change Any takers? I'm looking at you chat and you people looking we're looking at here from home If you want to submit a pr watch, I'm gonna get 10 prs all making this exact change um I think css should be fine here um, although we do need to make sure that, um svg supports all the relevant css constructs Uh, I don't know that svgs Have full css support Oh, this is not many github notifications like If I go away for like two weeks, I have several pages Uh, which is a good problem to have but it also is like, you know, all these people are waiting They're not necessarily waiting on me Some of them are just issues that I watch because I'm interested in the progress on like some rust ticket, for example I shouldn't call them tickets tickets is very enterprising on rust issues. Um But like knowing that a bunch of people are Waiting on me or blocking on me is makes me sad um Yeah, so an example here is um So this switchable buffering for standard out is a a ticket in the standard library that I've been watching for a while It's a it's a pr To make it so that you can choose to make standard out Um Be let me link to the actual issue. Um You can make the You can make standard out not do line based buffering if you want to if you just want sort of raw standard out Um And it's a it's not something I need very often But I like to keep in touch with somewhat like the deeper details of how stuff works under the hood here And this is a good example And you know, it was open in october of 2020 It's it's been going on for a while because a he or two is not entirely clear what the right api should be like Um It's pronounced dray man. Oh dray man nice um But yeah, so this is like an example of something that is you know, uh It shows up as a notification, but it's not actually blocking on me. Um All right, so let's do the haphazard one just to sort of skip to another project. So haphazard is the yet another one of the streams that we've been doing which is um Implementing hazard pointers a library for hazard pointers in rust um And one of the things that I did since the last stream that I did more sort of asynchronously offline Was try to tidy up the interface so that it's a little bit nicer to use um You know giving a little bit higher types like atomic pointer that that behind the scenes just deals with the fact that like makes everything box for you and stuff um I recognize this name I've been working on a concurrent hash map with the help of haphazard and have seemed to come across undefined behavior and safe code They're probably right. Um I believe them already All right, so they have a family struct they use atomic pointer They create an atomic pointer to null They create a new And an old And they compare exchange Where they expect the current value to be old and the new value to be new um, this will fail because The current value is null. It's not old And so therefore this will fail and I forget what I set the cement. I I think I see where this is going already. Um So if we look at atomic pointer and we look at compare exchange week It returns an option replaced And if it fails It returns you Right, so the idea here is if the compare exchange fails, then we want to return new because We never actually stuck it. We never actually gave it away We never actually put it in a shared location and therefore it's safe for us to give The caller back the new that's what this isn't supposed to be um And I'm guessing that this is not true. That's what I think this issue is going to actually be When run with cargo mirror run, this produces Box unique new unchecked raw Right, so this is from box from raw, which is in practice what p from raw here actually means Safety common for atomic pointer new says that p must be a valid reference to t or null However, when the compare exchange fails inside compare exchange week The null pointer ends up in the error variant. Oh, so pointer here ends up not actually being New pointer here ends up being Ends up being the the old current. No, you're totally right um Yeah, so I think where this is actually broken is um So if we go over here to lib The lib here is too long. I should break this into a separate file um weak pointer Yeah, because the semantics of a compare exchange in the standard library is If it fails it returns It doesn't return the thing you passed in as current It returns what the value actually was Given that it wasn't current The return values result indicated by the new point it was written and containing the previous pointer this should arguably be The current pointer is a better way to frame this um So the actual fix to this is let me I'm going to first open this separately because I'm going to want to link to this in the response, but um The fix to this is that Here new here is a raw pointer and raw pointers are always copy. So we can just use new here That's the correct answer um, I so I think the answer here is uh l 6 1 6 um That's just straight up wrong uh The bug is in um compare. Oops Make that ooh, what am I doing? I was trying to make this larger, which I failed at uh the bug is in compare exchange weak And I suspect also compare exchange um a exchange assuming that uh the Pointer version returns Current on failure when in reality it returns, uh the value stored in the standard sync atomic atomic pointer That wasn't current Uh The solution here is luckily simple We just need to change This and this is where I think this Uh in order for this to be embedded correctly by github I'm going to press y and that way I get a commit specific url and that way it's going to actually fill in the code block here um Uh And then I'm going to say instead We're going to change that to Who was never shared and was a valid p So this should just be new And this can be a move Which should work since new is just a star mute t and That is copy Want to file a pr? For this and the non weak variant Uh and what I'll also say is um We may also want to update the wording on the pointer method, uh the the documentation wording on these methods to uh Clarify what previous value means maybe it should say Uh I don't know what it should say Here for example Uh, let me link that issue here too Um So this is some discussion chat about um, where to discuss things. So whether you should discuss things in, uh GitHub issues or Whether you should discuss things in like discord where you can have more real time whether you should try to have a voice call You know, it's tricky I I do really like having these discussions on github because it makes it more permanent It means that there's sort of a track record of the discussion that's easier to follow Sometimes though, it's true that it is useful to Um sort of actually have the back and forth But even that, you know, if if it's in the issue, it's something you can link to you can link to individual comments Um, so it it is a little slower to do it this way, but it can be It's much more valuable sort of his In the future it will be more valuable um And that doesn't mean that you could never take these discussions on discord Like if you feel like you're really just bike shedding something and you need the back and forth Do it somewhere else and then come back and do summaries in the issue that works pretty well voice chats are hard like Both because you're strangers on the internet, but also because Once you put either sort of synchronous voice or worse yet video You feel put much more on the spot and I think for some people that's just like Uncomfortable either because they don't feel confident expressing themselves that way or they worry about saying something wrong Or they just worry about the other person being an asshole, right? Like It's easier to in text Measure out your responses and articulate your arguments. Well So I do see the desire to to not go all the way and actually have, you know, real conversations Sometimes that is useful, but but it's pretty rare that I take I take someone up on an actual conversation like that unless there's someone that I've actually been interacting with a lot Internal so I think for development within a team It's very different right if you work with if you have co-workers that you deal with on, you know GitHub issues or something I do calls with people like that pretty regularly because it is really valuable but doing it as like I'm just going to do this with a random stranger that I'm collaborating a little bit with online is I think the bar is much higher and I don't want to put that pressure on them That would be if I reach out if they reach out. I'm more open to it partially because I'm I generally am pretty okay with just, you know, doing things on on the spot But even then, you know, it's it's more of a time commitment. It synchronous communication is pretty costly and and it's not something that You should generally I think ask of other people unless you have a pretty strong reason for doing so Yeah, and conferences and stuff are a great way to have Those conversations and a little bit more of like a batch setting One of the many reasons why I miss conferences Okay, so Troy is on stream. I try That's funny. Okay, great. So we got a response to that. I'm going to mark this one is done People should not be afraid to talk over voice In person used to be the only way to communicate That's true But even though they shouldn't that doesn't mean that they're not And I think, you know, there's like the reality of the situation that That's not everyone's preferred way of communicating and that's okay I don't think it's necessary and I think there are some serious upsides to doing it where There's sort of a log of the conversation too Let's see Okay What else we got inferno move from lacy static to one cell Okay, so this is a These are two crates that you you may have heard of both lacy static is one that just like is everywhere in rust like it used to be basically the only way you accomplished what lacy static does which is It allows you to have a static variable in your program Whose constructor is not const And they used to be much more important because so many things were not const, but even now, you know, um This is like let's say you want to create a mutex and you want to stick it in a static variable The way you have to do that today is using lacy static. That is what's cool is that's changing with rust 163 Like the next release where mutex new is actually const now, which is some awesome work that the mara has been doing But but there are a bunch of contexts where the constructor for your Your static variable you basically want to Only run once your program is run for the first time The the idea here being that The constructor for a static thing needs to be const because You're basically going to run it at compile time and then stick the resulting like bits into the binary So when the program runs for the like when the binary runs It can just load those bits into a predefined version of The program memory at startup and not run any code to initialize it What lacy static does is basically When your program runs, there's a little bit of a like a secret section in the binary of code that gets to run Before like main sort of and lacy static is sort of takes advantage of that where it It runs the setup code for your statics in there So basically it makes the statics just be a blob of uninitialized memory And then in that pre-main little initialization Code path it It runs the code to construct the thing and then Fills the bits into the uninitialized memory and marks it as initialized so that from that point forward You can access it and because it is in static memory It continues to be a valid static One cell is a replacement of lacy static that that changes the api a little bit It's also actively maintained which is nice But but one of the reasons why it's nice is because you have control of when the initializations happens So rather than just saying it runs in this like pre-main init bit What you actually do with one cell is when you access the variable like when you access the static You also say how to initialize it and that is the point at which it gets initialized So you can think of this as it sort of gets lazily initialized, but only once hence the name one cell And so what this pr is doing and I haven't looked at the diff yet is Moving from the lacy static crate, which is only passively maintained to the one cell crate which generally has an api that's considered Better it's also a little faster and it's well maintained I think one cell is also on path on a path to being standardized in the standard library. There's a pr open for it. I believe that's been open for a little while So let's see what this changes This does have to change like a little bit because as I said, it's not a slot in replacement It actually has to change where the initialization happens Let's see Okay, this is that simple Right. So in this case we use the lacy static for keeping a track of the number of threads And we do this because we have some there's some part of the data structure where we want to shard it by the number of threads So that you get more efficient concurrent access because the threads don't all access the same variable Instead they access a sort of per thread variable And then other threads have to read all of them So writes or things that need to access all of them are slower But most operations you only need to access one and so there's less contention on any one value But the number of threads we sort of assume doesn't really change or the number of cores rather or the number of Thread slots is the right way to phrase it So calling numcpu's get is a little costly It has to do a bunch of like operating system lookup calls and stuff So we want to cache that value and we're going to be accessing it everywhere. So we would rather Have it be in a static than having to pass it around our program everywhere Uh, so this here declares as a static uses the lazy type from one cell Oh, that's nice. Okay, so It seems like Oh, I didn't even know that one cell had this. That's nice. So that means the diff is actually going to be pretty small Uh, one cell has two APIs. It seems so there's one cell which has new that gives you a thing and Uh get Where's the Yeah, so and get or in it on that which is going to initialize it if it doesn't exist with by running the closure And then it also has lazy, which is a convenience type to streamline the pattern. Uh, and so that is When you call new which is a const fn then you pass in the closure to run and this presumably then uses The the pattern we talked about of Using the sort of in it section um And and runs this code for you before main So this is just to encapsulate that particular pattern Nice So that means the diff here is actually going to be pretty small Because we can just say when we construct a lazy that the constructor is numcp use get So the first time someone tries to access this variable run this function Oh, you're right. It doesn't use life before main. You're totally right. Um, it It sticks the value in there where when you access when you call get on this value Then it checks if it has been initialized and if it hasn't been initialized then runs the initializer You're totally right. It doesn't run before main. There is a way to run things before main But this is not using that you're you're totally right Um So this is what that is doing is it's creating a one cell for you where Calling get on it will use this as the initializer if it hasn't already been initialized Um So that's nice and easy This is n threads as a string. So that's just the same This is also just the same This is the same And this is If multi threaded Right. So this could technically be Uh, you know, this is a tiny knit, but this could just name the function similar to what they did in all the other files I don't know or I guess it was just in the first. So you see up here Here they used this syntax where you just give the name of the of the function And here they gave a closure that immediately calls the function So that could just have used the same form as above. I'm not even going to comment on it. It's not quite worth it Um What if more than one thread tries to initialize it the first time as internally there's Like internally both of these crates have a mechanism for detecting And making sure that only one thread does the initialization and the other threads will wait Um Why use format instead of two string Good question I don't think there's a particularly good reason But that's sort of unrelated to This particular pr if you want to submit a pr that fixes that absolutely, please go ahead I love cleanup prs because they're really easy to review They make my future life easier and they make me happy. So like Absolutely submit prs that are just sort of tidying up things um Why is this even a lazy static? Oh, it's because so here the constructor is a ye the constructor here is a const but uh Because this same static we define under conditional compilation It needs to have the same type because otherwise You know If you have the configure if you don't have the config every place that accesses it like if it's a one cell You need to use get if it's not a one say you don't need to use get And so therefore we just have them be the same type in both cases Which means that we need to use the the lazy initializer here um So that's fine This is Right, this is in tests where we want to use the same input for all the tests. We just stick it on a static I don't know that there's a particularly compelling reason to use the static, but that's fine This is I mean, it's just converting what was already there. So I'm not gonna Talk about that too much. This seems fine Oh, this had incorrect indentation It's because it's in the macro And this use is two strings. So we're clearly just inconsistent beautiful, thank you merge So this was in inferno. No I really need to The reason I run git lug one line here is I want to see whether there are other changes Since the last release I did Um, it's a typo fix. I don't I don't care too much about that So for the change log Arguably, I should just use something like cargo release here So I don't have to do this stuff manually, but I don't know. There's something kind of nice about doing it manually I don't know. I don't really mind it So here we're gonna say 0 11 5 was released 2022 0 6 18 Changed Moved from lacy lacy static to once cell And that was through pr number 249 Check the cargo tumble Sometimes what I'll also do is when I do a new version release I'll also run, you know cargo update Cargo outdated r Just to see whether there are any dependencies that I should move to a new major version Usually, I mean only if they're internal dependencies or private dependencies only, right? But just because you know, there's no reason not to bump those while we go so we don't have to do it later In this case, there are none. So the diff is easy enough You'll notice here. I start naming the new tag because I'm about to tag this with the correct tag Beautiful, all right, so we're just gonna say release 0 11 5 Run my little script Push tags get push cargo publish. There's an argument here for I should wait for ci to pass in fine Just to see that I'm not, you know Doing something very silly. I'm gonna run cargo tests ci already passed before I merged the cr right so the the pr so this shouldn't This really shouldn't matter Is it okay to nitpick code and consistency or quality if I'm the maintainer or is it considered rude? I would have a hard time accepting a pr without leaving knit comments, but my ocd could yeah, so it's a good question me You know it could be considered mean What I try to do is I try to be very clear in my review Whether something is a knit that I'm willing to ignore Or whether it's a block or for merging and I try to keep those straight in my mind What I'll sometimes do is If there are other changes like if there are changes I require to the pr that are not stylistic changes then um you know, I'm More inclined to say also fix the stylistic changes If there's only stylistic changes left Then I tend unless they're like particularly egregious I tend to just merge it and then just do those fixes myself after because it's it saves the back and forth And it makes that person feel better And it saves me time too because I don't need to keep going back and forth on it um, that said for stylistic things If it's actually style like if if it's code formatting rust format should take care of that that should be run in ci If it's like this is the wrong name for this type those things I will actually comment on because I care a lot about the public interface of my crates whether that be the documentation the types the method names So those I will comment on because I don't think they're nets. I don't think they're unimportant um But you know for for anything else, you know, I I ci for me runs clippy and rust format and they have to pass and if they don't I tell them It doesn't pass because it doesn't pass clippy or rust format. So that's what they have to do um But but it is a balancing act and and I think the the thing that you train yourself on is Where's the bar for when I say this is good enough? I can fix up the rest myself What I will sometimes do especially if I see that they're a new contributor is When it gets closed when it's like like the 90 mark or something where all the important bits are in place What I'll do is usually Merge sometimes I'll leave the comments to say hear a couple of things But even that is often not important because it becomes so nitpicky So I'll sort of approve the pr merge fix myself and then go back to the pr and say um, you know something along the lines of I decided to do a few more uh stylistic tweaks or or tweaks to Fit the consistency of the current code base a little better if you're interested Take a look here and then I link them the commit or the commit range That I just pushed so that they can see what I did differently And the reason why that that sort of closing that loop is important Is because imagine that they want to do another contribution later You want them to learn if you just do the fix on your own If they're one-time contributors not a problem, but the second time they come around you're gonna have to do that yourself again So you would rather them learn But this way you take away the cost from them for a one-time contribution or first-time contribution But you do still give them an opportunity to learn going forward um So so yeah, I think It's not there are no right answers here, but in general I would say If you can group it with other changes that are required Then absolutely try to have them make that changes if you feel like there are too many of them try to do only a few for each round of review and Keep in mind the level of at what point should I just do the rest myself? Um, and then make sure you close the loop and link them back to it Uh, great. So we did a new release of this. So I'm gonna say Published in 0.11.5 Comment And again remember to leave thank you messages remember to leave messages about Again, this person took time out of their day to send a pr to my thing so that I didn't have to do it so Even if their change is like small It saved me a bunch of time. I didn't have to go do this I probably it saved the people using this library a bunch of stuff because I wouldn't have gotten around to it for ages and also tell people what version the change comes out in because Six months from now someone's gonna care um All right done See what we got left Uh close close close What do we got left here? Uh, so this is more for open ssh This is more a design question around splitting low level and high level I'll I'll deal with that separately because I have to think about that um A closed pr that's always interesting This is so this is a a draft pr for inferno So inferno supports Bringing in profiler data from a bunch of different sources. So it supports perf It supports detrace it supports uh Couple of other like the x code output format and someone wanted a while back phpx debug traces and You know the the way you do this is you add a a Collapser which is the ingest pipeline that turns the profiling data into a format that the Uh visualization generator can understand And I remember this starting up I guess 2019. Yeah, this draft started and We went through a couple of reviews Because there were a bunch of things that were like not quite right or there were some edge cases that weren't captured Uh, and ultimately I think this person ended up moving on from The pr they didn't have a chance to come back for a while um and I It looks like they recently discovered that they don't care anymore Um, which it might be, you know, they don't use php anymore or something. Um, so they're going to close the pr So heart because Thank you for offering to do more work And thumbs up for understood That seems good and I'll say Closing seems reasonable and then Should someone need this in the future? They can always pick up from where you left off Oh, yeah, it's true. You can nitpick a lot more for Um, you know company internal or private repositories or or places where you know other people That said even there I think you should keep the same things in mind. You might have junior team members um, or You know down the line someone might come back to look at this To figure out why a change was made or even just like you're up for promotion and the senior people above you are going to evaluate What are you like in reviews? Like do people enjoy working with you or collaborating with you on code? And if they see that you're just endlessly nitpicking on everyone's crs I keep calling them crs because internally in aws, they're called crs. Not prs for code review So they're synonymous um but Ultimately, like you should still a be nice to people but b make sure you use their time and your time wisely, right? Sometimes it's just not worth the extra round trips And either you should do it yourself and then offer, you know Here's the commit where I made the last few changes myself to to save us both some trouble But you can still learn from it But also don't You don't want to end up being the person that everyone's like I just don't want john to Review my code because I know that it's just going to be pages and pages of nitpicking, right? I don't want that either um Great. All right, so that's done Let's do maybe one or two more Ooh different project. Let's do that Oh, okay. I already know this is going to be fun. It has taiki and ralf Who recently commented? Okay. This is great um Update dependencies fix on soundness nci failure from december 2020. All right. Where did this go? uh miri reports undefined behavior So in 2020 I was like I don't know what this error means whether that whether it's miri that's broken This is when stacked boroughs were in the earlier stages Is this miri being wrong? Like is it a false positive? Is it a bug in in bus? Is it a little bit of both? um That's funny. I left a review in december 2020 and two days ago taiki made the change That's great Yeah, so this is something that I think I may be weird in this. I really like having explicit drops for Indicating that you shouldn't be using a thing anymore even though it doesn't semantically make a difference or performance Why makes a difference it just Maybe it's like the the literal Literate programming part of my brain that's just like I feel like this should go away now because it's confusing if it keeps living um So I guess this commit restores Yeah, that's fine. Um Okay, so there was a Miri limitation here somewhere Ah, so okay, I think I remember what happened here it's um Part of what we do in bus so bus is a broadcast channel the idea being that you have um It's sort of the opposite of the channel you have in the standard library You have one sender we have multiple receivers and furthermore it's broadcast So any send goes to every receiver Uh and the message doesn't go away from the channel until every receiver has seen it So if like if the sender sends one two three every receiver will receive one two three um and in that order And one of the things that was needed for this library was a way to Drop values um That have been received by all receivers And I think when what I ended up doing was spawning a separate thread to do that dropping But I never wait for that thread to finish That thread just sort of keeps running in the background And it just assumes that when your program exits that thread goes away And it doesn't cost anything because it's just blocking if there's nothing to do um And Miri doesn't like it when you have threads to just keep running because it's basically a leak Uh, I think that's what we ended up at here Uh Yeah, right. So Miri checks for thread leaks But that's also tied to memory leaks because if you have a thread leak anything that that thread Owns also gets leaked as as in memory um Right, so I think that's where we ended up and the reason why this sort of stalled Was because this was a thing that needed to be added to Miri um Miri current oh interesting. Okay, so it started up because there's an issue in crossbeam utils And this package depends on crossbeam utils. So ideally we should update crossbeam utils um Which is what caused ci to fail in the first place through miri Miri currently fails in various projects to issues although it's Probably possible to work around them by disabling weak memory emulation preemption This crate is not affected by that issue. Okay, that's good to know Uh This crate depends on atomic option with vulnerabilities that could affect this crate. Yeah, I haven't looked at this in ages Uh, ci uses 1604 which is not only supported Uh, great Ci is now green except codecub, which is angry that the comments I added were not covered. Yeah codecub is very Um aggressive part of this is because it doesn't use the new lvm Instrumented coverage it uses the um Like profiling based coverage like the the sampling based coverage. So it's not perfect Uh, and then ralph looks in and like why do you need to disable isolation? uh, and we do that because There's a received timeout function which relies on time and that means it can't be isolated Um, which arguably means I should just hide receive timeout from the execution of miri Um Great, let's look at the actual diff So this bumps parking lot core to zero nine. It removes atomic option. It bumps cross beam channel great Azure pipelines parking lot core went to latest Grab miri install rust Run miri with disable isolation ignore leaks No preemption no weak memory emulation. That's fine Both of those are due to miri issues preemption I see so this is a standard library miri interaction which ends up affecting us Uh, the few tech things apparently been fixed When has it been fixed? Merged 14 hours ago. Okay, so that means We could probably get rid of The weak memory emulation, but that's fine. I don't I'm okay with landing that without it. I'll just leave it this unresolved bumping latest bumping that that seems fine This Skips an example from miri that seems fine Why this Hey, we used to drop the state outside of there And now we drop the state Inside of here because why I wonder about this there's got to be a change somewhere. Okay, can we I want this to go away Here we go I need some more context for that change. Oh, I see it's because we're using Our own atomic option which is No, it has a swap What's the remove all annotations a there we go If all does know the none Oh, I see. Okay. So this is just a very trivial and obviously correct implementation I should never say obviously correct for something like this, but seemingly clearly correct implementation Of atomic option that doesn't rely on the atomic option crate And it only provides the operations we care about which are swap and take So that's fine and because that api changed replace now becomes swap replace none becomes take and This becomes Very interesting. Why did this have to change? Why did this drop have to go inside of here? I don't understand that part of the change So State here is a reference I think that I think this didn't need to be moved Um because It's still safe to access The inner value all the way until this fetch add. So this Uh, this part of the diff isn't necessary. It's not wrong, but it could have stayed where it was um Yeah, my the the comment I've hidden here is just that um The there was a previous version of this pr where They just removed this drop entirely because it's not necessary And I said I would like to keep the explicit drop because I want to signal that you can't use state anymore And I think what happened was when they restored it They restored it in the wrong place because they restored it where they remembered it being rather than where it actually was um It said something about putting the drop in the else branch kind of like Simpsons is my love of that Oh Right, right, right. No, you're totally right. Um, if we go back to here Trying to re borrow for shared only Right miri complains. It's not actually okay. Yeah, I see what happens. Um, it's because Trying to drop it out here means Uh Also trying to access it after we've Mutably taken a reference to it here Which is not okay, but accessing in the else branches. Okay. That's fine. Yeah. Yeah So this is this is a correct change that makes me very happy great. No, you're totally right. Uh, okay So this seems great Thank you for following up on this after all this time Heart And I don't care about the the patch change. That's fine. I also want to move this to use github ci because the uh The azure stuff is all annoying So i'm going to merge that uh, which is going to be uh, dot dot slash dot dot just minor slash bus um And so i'm going to do this as This can be a minor release to be honest. There's no new feature in this at all Um, there's all sorts of like bus is a fairly old crate at this point arguably, you know I should update it to the 2021 to 2021 edition. I should move it over to use github ci I should get rid of the badges, but I it's not important Like what matters is fixing the Uh, the issues in it. Uh, I don't think there's even a change log for this Uh, so i'm just going to do git commit dash a something like bump dependencies Um bump important dependent important dependency bumps Release version I can't even Get commit amend 224 release 2.2.4. Um, this bumps multiple important Dependencies, uh, and gets rid of the atomic option dependency Um And if we tag this Uh, there's a bunch of old tags here. That's fine get push tags and cargo publish Amazing 2.2.4 is out And this is done. So I'll say released in 2.2.4 Um, the reason to remove the badges from cargo tumble is they've been deprecated They're no longer a thing that they recommend that you put in cargo tumble and the reason was because They realize people put it in the read me anyway and crates.io renders your read me by default now So you just ended up with the badges in two places and The badges on crates.io needed like special code to support them because you needed Like for each badge kind you needed code in crates.io to generate the appropriate badge So it was just like we just render the read me and allow people to render their badges as they want Um Okay, I think we've now sampled a bunch of different crates. We've also released three new versions of different crates Um, and it's getting pretty late over here. So I think that's where I'm going to cut it Um, any questions sort of at the tail end here before we end for today Um, how would I know if I need to use miri as ci? Um, I would use miri as ci for anything where you have unsafe code Uh, or where you're doing Unsafe code is the the primary indicator, I think but anywhere where you're doing Like concurrency or synchronization Miri is also really useful because chances are under the hood You're using a library that does unsafe stuff around that And miri is pretty good at catching the undefined behavior even through those layers of abstraction um Totally unbiased but this is a great time to stream Yeah, I I want to do more streams that are friendly to like time zones that it's not just like Europe and the you are western europe and the us Um, it's just it it's a little harder for me to plan around Um But it's I want to pick more times like this too and it's good to know that there are people here who will come watch um Oh, you mind scrolling up and making a comment of my question about staying motivated to learn um During periods of discouragement Uh, how do you avoid, you know getting? Your curiosity stifled with all the stuff you have to deal with, you know I don't know that I have great advice There are times when I get, you know, tired of the work that I'm doing or tired of the open source stuff Or tired of, you know, following up on prs all the time and and feeling like, you know, I'm at this point I'm just catching up all the time rather than working on things that I think are new and interesting And that's sort of the the curse of doing well in some of these areas, right? Like if you write a library that gets a lot of uptake That means that a lot of people are using your thing Which means you need to spend a bunch of time on it to To address issues and add new features and whatnot. Um I think that the the thing that has helped me the most is a to realize These people don't have a claim on my time. I I want to help the people who use what I build But it's still ultimately, you know, essentially me volunteering my time And so it's okay for me to not work on them. I think one thing that helped me a lot was being like I'm just not going to respond to things immediately And say, you know, I'm going to do three hours every weekend or something And that just means that people will have to wait a week before I get back to them And it sucks, but that's the reality of what being a sort of volunteer maintainer is like The second thing I would say is try to aggressively get co-maintainers And I don't mean aggressively isn't like that's how you should talk to them But more like if someone submits a pr and it seems like, you know, they're interested They have a good use case for that seems like they're they know what they're doing or they're willing to learn Give them permissions like make them a co-maintainer Maybe don't give them like publish permissions to crates that I owe immediately But bring them on board try to engage them in conversations like when there are issues or prs Loop them in like if there's something where you're like, I'm not entirely sure what the right design decision here is Like CC them like at them in that discussion thread to try to get their input Expose them to more of the project So this happened to me with the iMap crate I maintain for example There was a person who reached out and was like, you know, I would love to Just get more involved with this crate and I was like great Let me start tagging you and things let me start like if there are issues that I don't have the time or I don't know I don't either have the time to follow up on or I know I won't do it for a long time Let me give you some pointers try to do some mentorship and that would great Like I loop them in whenever there's a design decision to be made And then they start making contributions themselves and like Make progress towards issues that are tagged as the milestones for the next release Same thing happened with the open ssh crate where Nobody's you just like really stepped up to the plate. It was like pr after pr after pr Fixing issues responding to people filing issues and and that helps me a lot too Not just in a technical sense, but in like a Feeling better about my work because I know there are other people Dealing with it. There are other people addressing it helping with it And it's not all on me and taking that weight off helps like my Mental sanity too and and and also my willingness to keep going on the other things that I have and I think this is a lesson for the people who aren't maintainers too like Maintainers have a lot on their plate. So if you are willing to step up and help a project like please reach out I would love more people to do it unfortunately, it is the case that people Are often enthusiastic in the beginning and then sort of loose interest over time Which is understandable, right? I would have done the same if I didn't own the project But but try to like keep up or give some indication of what kind of time commitment you're willing to give But know that it is appreciated like I get people who contact me on issues Or I will explicitly ask on a pr an issue like hey, would you be interested in like taking more ownership of this? I get people who email me or tweet at me or whatever and I I love the chance to Give away more parts of of the things that I end up being the maintainer of because I know that I can't be A good responsible maintainer for all of these things if I do them all alone That's the best I can the best I can say If you have a crate that you don't want to maintain anymore that one's harder because You don't really you're not really in control of giving it away There is always the option of just like deprecating the crate and say, you know, this is gone now I'm just not going to maintain it. Um, and if people go, well, why not? I need it Then you say, well, here you go. I will give you all the permissions There isn't really a path for like I'm going to look for someone to give it away to that doesn't really happen The closest you can do is like you can add, uh, rust bus as a maintainer Which is like a group of people who have volunteered to sort of handle Be the backstop in a sense for these projects, but I don't know that that's meaningfully better than just saying, you know This is at this point unsupported un-maintained and if you're interested reach out to me and we can chat Put that in the read me and that's just where we're at Um Would you ever consider full-time operating? Uh, offerings when I see os I started reading operating systems and it's os s and it's different Would you ever consider doing this, uh, full-time? Like, uh, again offering some open source software development Maybe it's hard to know whether it's sustainable. I think the The path towards it would probably be more of a sort of combination of teaching consulting open source maintenance and Um And maybe something along the lines of like Uh build on demand which falls under consultancy But I could imagine that a combination of those might actually fund me full-time probably not for a few years, um But that would be really fun if if I was actually able to like Maintain these as a full-time thing and get sufficiently paid for it I would probably come through something like, you know, github sponsorships And maybe foundation fellowships something like that. Um, I think I would also love to do more streams more education more teaching maybe do like develop a rust class of some kind Um And also I could imagine doing, you know, a consultancy where I take on both, you know Hey, we're a company and or a project and we need someone to build a crate that does this Will you build it for us? Sounds like a lot of fun. I get to work on different projects Well, there would be more of a handover than me than maintaining it forever. Um And a combination of that and you know, we're a company and we're using rust Will you come help teach us or teach us about this thing? Or teach us how to use this library or whatever and I could do that as sort of a paid gig Maybe some combination of these is feasible to do full-time. I I don't know at the moment. No, but maybe in the future All right. I think that's where I'm going to cut it. Uh, thank you all for coming out. Hopefully this was interesting If it was then we'll do it again. If it wasn't I don't know whether I'll ever know but, uh I guess I guess the analytics will tell us afterwards. Um, but thank you everyone for coming out and uh, I hope it was good See you all Ooh, no, I don't want exit OBS. I want to stop recording